Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - add Res::clone #4109

Closed
wants to merge 1 commit into from
Closed

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Mar 5, 2022

Objective

Make Res cloneable

Solution

Add an associated fn clone(self: &Self) -. Self instead of Copy + Clone trait impls to avoid res.clone() failing to clone out the underlying T

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 5, 2022
@BoxyUwU BoxyUwU added the A-ECS Entities, components, systems, and events label Mar 5, 2022
@MrGVSV
Copy link
Member

MrGVSV commented Mar 5, 2022

Would this prevent the contained data from being cloned by deref?

let my_cloned_data = res.clone();

Like, would we need to do the alternate form:

let my_cloned_data = (*res).clone();

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Mar 5, 2022

the alternative form i think is actually (&*res).clone() but yes

@james7132
Copy link
Member

james7132 commented Mar 5, 2022

Not a fan of this change. The expected behavior of a resource that is clonable should give you a cloned version of the inner value. If we explicitly want to create a new Res<T> from another one, I would much rather be explicit about it and use Res::copy(res) or something similar (akin to Arc::downgrade and other similar smart pointer APIs that avoid using self to ensure that it's not in conflict with an deref'ed function).

Does calling res.clone() on a Res<T: !Clone> give you &T due to Deref?

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Mar 5, 2022
@mcobzarenco
Copy link
Contributor

Not a fan of this change.

Same here

The expected behavior of a resource that is clonable should give you a cloned version of the inner value. If we explicitly want to create a new Res from another one, I would much rather be explicit about it and use Res::copy(res) or something similar (akin to Arc::downgrade and other similar smart pointer APIs that avoid using self to ensure that it's not in conflict with an deref'ed function).

Indeed, this is what std does, the Clone impls for smart pointers (Rc, Arc etc.) clones the underlying resource via deref and you're expected to use e.g. Arc::clone to clone the pointer.

@james7132
Copy link
Member

james7132 commented Mar 5, 2022

Indeed, this is what std does, the Clone impls for smart pointers (Rc, Arc etc.) clones the underlying resource via deref and you're expected to use e.g. Arc::clone to clone the pointer.

Actually both Rc and Arc clone the pointer, not the value. Only Box clones both the pointer and value. Still, I expect the base Res<T> pointer to act like &T, and ultimately that's what enables the ergonomics around it.

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Mar 6, 2022

&&T also has the issue of .clone() not cloning the inner T so this is not actually limited to Res<T>: Copy + Clone but regardless I removed the trait impls and just added Res::clone since we ought to provide some way of doing this even if the obvious way of Copy is disliked

@BoxyUwU BoxyUwU changed the title make Res<T>: Copy + Clone add Res::clone Mar 6, 2022
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 22, 2022
@@ -220,6 +220,17 @@ where
}

impl<'w, T: Resource> Res<'w, T> {
// no it shouldn't clippy
#[allow(clippy::should_implement_trait)]
pub fn clone(this: &Self) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be #[inline] as a tiny method.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 30, 2022
@alice-i-cecile
Copy link
Member

After reading the discussion, I think I prefer the current design over implementing the trait.

@@ -252,6 +252,17 @@ where
}

impl<'w, T: Resource> Res<'w, T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just throwing an idea out there: maybe we should call this something else to avoid the ambiguity entirely?
(ex: duplicate())

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's my preference here too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm actually within the context of requiring Res::clone to resolve the ambiguity, I think thats nice and clear, given that we're cloning the Res. I think we should merge this as-is.

@cart
Copy link
Member

cart commented Sep 27, 2022

bors r+

bors bot pushed a commit that referenced this pull request Sep 27, 2022
# Objective
Make `Res` cloneable
## Solution
Add an associated fn `clone(self: &Self) -. Self` instead of `Copy + Clone` trait impls to avoid `res.clone()` failing to clone out the underlying `T`
@bors bors bot changed the title add Res::clone [Merged by Bors] - add Res::clone Sep 27, 2022
@bors bors bot closed this Sep 27, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
# Objective
Make `Res` cloneable
## Solution
Add an associated fn `clone(self: &Self) -. Self` instead of `Copy + Clone` trait impls to avoid `res.clone()` failing to clone out the underlying `T`
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective
Make `Res` cloneable
## Solution
Add an associated fn `clone(self: &Self) -. Self` instead of `Copy + Clone` trait impls to avoid `res.clone()` failing to clone out the underlying `T`
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective
Make `Res` cloneable
## Solution
Add an associated fn `clone(self: &Self) -. Self` instead of `Copy + Clone` trait impls to avoid `res.clone()` failing to clone out the underlying `T`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants